feat: Granular abac permissions#40408
Conversation
|
Looks like this PR is not ready to merge, because of the following issues:
Please fix the issues and try again If you have any trouble, please check the PR guidelines |
🦋 Changeset detectedLatest commit: a552d6b The changes in this PR will be included in the next version bump. This PR includes changesets to release 43 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
📜 Recent review details⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
WalkthroughThis PR introduces four granular ABAC admin permissions, gates ABAC UI tabs and contextual views by those permissions, updates routing and sidebar visibility to redirect to allowed tabs, enforces the permissions on ABAC HTTP endpoints, adds locale entries and a changeset, and expands end-to-end tests to validate 403 behavior. ChangesGranular ABAC Admin Permissions
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Suggested labelstype: feature 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #40408 +/- ##
===========================================
- Coverage 69.99% 69.64% -0.36%
===========================================
Files 3309 3317 +8
Lines 120956 121950 +994
Branches 21683 21824 +141
===========================================
+ Hits 84666 84934 +268
- Misses 32990 33691 +701
- Partials 3300 3325 +25
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
coderabbitai
Bot
added
the
type: feature
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/meteor/ee/server/lib/abac/index.ts (1)
12-14:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAwait permission creation instead of fire-and-forget.
On line 13,
void Permissions.create(...)suppresses errors and letscreatePermissionsresolve before writes complete. In initialization code, this risks silently missing permissions.Permissions.createis async and returns a Promise; elsewhere in the codebase (e.g.,ee/app/livechat-enterprise,ee/server/lib/deviceManagement) these operations are properly awaited usingPromise.all.Proposed fix
export const createPermissions = async () => { const permissions = [ { _id: 'abac-management', roles: ['admin'] }, { _id: 'manage-abac-admin-settings', roles: ['admin'] }, { _id: 'manage-abac-admin-room-attributes', roles: ['admin'] }, { _id: 'manage-abac-admin-rooms', roles: ['admin'] }, { _id: 'view-abac-admin-audit', roles: ['admin'] }, ]; - for (const permission of permissions) { - void Permissions.create(permission._id, permission.roles); - } + await Promise.all(permissions.map((permission) => Permissions.create(permission._id, permission.roles))); };🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/meteor/ee/server/lib/abac/index.ts` around lines 12 - 14, The loop currently fires Permissions.create(permission._id, permission.roles) with void which suppresses errors and returns before writes complete; change createPermissions to await all creations by mapping permissions to Promises using Permissions.create and awaiting them with Promise.all (or sequentially await inside the loop) so errors propagate and initialization waits for completion; reference the Permissions.create calls in the createPermissions initialization block and replace the void pattern with an awaited Promise.all([...]) of those create calls.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/meteor/client/views/admin/ABAC/AdminABACPage.tsx`:
- Line 38: The sync button should be gated by the same granular permission used
by the API—use the per-tab permissions from useABACTabPermissions() (the
tabPermissions object) to conditionally render/disable the LDAP sync button and
prevent clicks when the user lacks manage-abac-admin-room-attributes; update the
component logic that renders the sync button (the button handler that calls
/abac/users/sync) to check tabPermissions.manage-abac-admin-room-attributes (or
the exact property provided by useABACTabPermissions) and hide or disable the
button and short-circuit the click handler when that permission is false so
users can't click and receive a 403.
---
Outside diff comments:
In `@apps/meteor/ee/server/lib/abac/index.ts`:
- Around line 12-14: The loop currently fires Permissions.create(permission._id,
permission.roles) with void which suppresses errors and returns before writes
complete; change createPermissions to await all creations by mapping permissions
to Promises using Permissions.create and awaiting them with Promise.all (or
sequentially await inside the loop) so errors propagate and initialization waits
for completion; reference the Permissions.create calls in the createPermissions
initialization block and replace the void pattern with an awaited
Promise.all([...]) of those create calls.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c29cda8e-13b6-4fd7-aac2-39fd861f1100
📒 Files selected for processing (10)
.changeset/brave-fans-tie.mdapps/meteor/client/views/admin/ABAC/AdminABACPage.tsxapps/meteor/client/views/admin/ABAC/AdminABACRoute.tsxapps/meteor/client/views/admin/ABAC/AdminABACTabs.tsxapps/meteor/client/views/admin/ABAC/hooks/useABACTabPermissions.tsapps/meteor/client/views/admin/sidebarItems.tsapps/meteor/ee/server/api/abac/index.tsapps/meteor/ee/server/lib/abac/index.tsapps/meteor/tests/end-to-end/api/abac.tspackages/i18n/src/locales/en.i18n.json
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: cubic · AI code reviewer
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
**/*.{ts,tsx,js}: Write concise, technical TypeScript/JavaScript with accurate typing in Playwright tests
Avoid code comments in the implementation
Files:
apps/meteor/client/views/admin/ABAC/AdminABACTabs.tsxapps/meteor/ee/server/lib/abac/index.tsapps/meteor/client/views/admin/ABAC/hooks/useABACTabPermissions.tsapps/meteor/client/views/admin/ABAC/AdminABACRoute.tsxapps/meteor/ee/server/api/abac/index.tsapps/meteor/client/views/admin/sidebarItems.tsapps/meteor/tests/end-to-end/api/abac.tsapps/meteor/client/views/admin/ABAC/AdminABACPage.tsx
🧠 Learnings (6)
📚 Learning: 2026-03-16T21:50:37.589Z
Learnt from: amitb0ra
Repo: RocketChat/Rocket.Chat PR: 39676
File: .changeset/migrate-users-register-openapi.md:3-3
Timestamp: 2026-03-16T21:50:37.589Z
Learning: For changes related to OpenAPI migrations in Rocket.Chat/OpenAPI, when removing endpoint types and validators from rocket.chat/rest-typings (e.g., UserRegisterParamsPOST, /v1/users.register) document this as a minor changeset (not breaking) per RocketChat/Rocket.Chat-Open-API#150 Rule 7. Note that the endpoint type is re-exposed via a module augmentation .d.ts in the consuming package (e.g., packages/web-ui-registration/src/users-register.d.ts). In reviews, ensure the changeset clearly states: this is a non-breaking change, the major version should not be bumped, and the changeset reflects a minor version bump. Do not treat this as a breaking change during OpenAPI migrations.
Applied to files:
.changeset/brave-fans-tie.md
📚 Learning: 2026-03-27T14:52:56.865Z
Learnt from: dougfabris
Repo: RocketChat/Rocket.Chat PR: 39892
File: apps/meteor/client/views/room/contextualBar/Threads/Thread.tsx:150-155
Timestamp: 2026-03-27T14:52:56.865Z
Learning: In Rocket.Chat, there are two different `ModalBackdrop` components with different prop APIs. During review, confirm the import source: (1) `rocket.chat/fuselage` `ModalBackdrop` uses `ModalBackdropProps` based on `BoxProps` (so it supports `onClick` and other Box/DOM props) and does not have an `onDismiss` prop; (2) `rocket.chat/ui-client` `ModalBackdrop` uses a narrower props interface like `{ children?: ReactNode; onDismiss?: () => void }` and handles Escape keypress and outside mouse-up, and it does not forward arbitrary DOM props such as `onClick`. Flag mismatched props (e.g., `onDismiss` passed to the fuselage component or `onClick` passed to the ui-client component) and ensure the usage matches the correct component being imported.
Applied to files:
apps/meteor/client/views/admin/ABAC/AdminABACTabs.tsxapps/meteor/client/views/admin/ABAC/AdminABACRoute.tsxapps/meteor/client/views/admin/ABAC/AdminABACPage.tsx
📚 Learning: 2026-05-06T12:21:44.083Z
Learnt from: juliajforesti
Repo: RocketChat/Rocket.Chat PR: 40256
File: apps/meteor/client/components/CreateDiscussion/CreateDiscussion.tsx:121-149
Timestamp: 2026-05-06T12:21:44.083Z
Learning: Field wrappers in rocket.chat/fuselage-forms (Field, FieldLabel, FieldRow, FieldError, FieldHint) auto-create htmlFor/id associations, aria-describedby, and role="alert" for errors. Do not manually set htmlFor, id, aria-describedby, or role attributes when using these wrappers. This automatic wiring does not apply to plain rocket.chat/fuselage components, which require explicit ID wiring per the accessibility docs. In code reviews, prefer using fuselage-forms wrappers for form fields and verify there is no unnecessary manual ID/aria wiring in files that use these wrappers. If a component uses plain fuselage components, ensure proper id wiring as per docs.
Applied to files:
apps/meteor/client/views/admin/ABAC/AdminABACTabs.tsxapps/meteor/ee/server/lib/abac/index.tsapps/meteor/client/views/admin/ABAC/hooks/useABACTabPermissions.tsapps/meteor/client/views/admin/ABAC/AdminABACRoute.tsxapps/meteor/ee/server/api/abac/index.tsapps/meteor/client/views/admin/sidebarItems.tsapps/meteor/tests/end-to-end/api/abac.tsapps/meteor/client/views/admin/ABAC/AdminABACPage.tsx
📚 Learning: 2026-02-26T19:25:44.063Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 38778
File: packages/ui-voip/src/providers/useMediaSession.ts:192-192
Timestamp: 2026-02-26T19:25:44.063Z
Learning: In the Rocket.Chat repository, do not reference Biome lint rules in code review feedback. Biome is not used even if biome.json exists; only reference Biome rules if there is explicit, project-wide usage documented. For TypeScript files, review lint implications without Biome guidance unless the project enables Biome rules.
Applied to files:
apps/meteor/ee/server/lib/abac/index.tsapps/meteor/client/views/admin/ABAC/hooks/useABACTabPermissions.tsapps/meteor/ee/server/api/abac/index.tsapps/meteor/client/views/admin/sidebarItems.tsapps/meteor/tests/end-to-end/api/abac.ts
📚 Learning: 2026-02-26T19:25:44.063Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 38778
File: packages/ui-voip/src/providers/useMediaSession.ts:192-192
Timestamp: 2026-02-26T19:25:44.063Z
Learning: In this repository (RocketChat/Rocket.Chat), Biome lint rules are not used even if a biome.json exists. When reviewing TypeScript files (e.g., packages/ui-voip/src/providers/useMediaSession.ts), ensure lint suggestions do not reference Biome-specific rules. Rely on general ESLint/TypeScript lint rules and project conventions instead.
Applied to files:
apps/meteor/ee/server/lib/abac/index.tsapps/meteor/client/views/admin/ABAC/hooks/useABACTabPermissions.tsapps/meteor/ee/server/api/abac/index.tsapps/meteor/client/views/admin/sidebarItems.tsapps/meteor/tests/end-to-end/api/abac.ts
📚 Learning: 2026-02-10T16:32:42.586Z
Learnt from: tassoevan
Repo: RocketChat/Rocket.Chat PR: 38528
File: apps/meteor/client/startup/roles.ts:14-14
Timestamp: 2026-02-10T16:32:42.586Z
Learning: In Rocket.Chat's Meteor client code, DDP streams use EJSON and Date fields arrive as Date objects; do not manually construct new Date() in stream handlers (for example, in sdk.stream()). Only REST API responses return plain JSON where dates are strings, so implement explicit conversion there if needed. Apply this guidance to all TypeScript files under apps/meteor/client to ensure consistent date handling in DDP streams and REST responses.
Applied to files:
apps/meteor/client/views/admin/ABAC/hooks/useABACTabPermissions.tsapps/meteor/client/views/admin/sidebarItems.ts
🔇 Additional comments (9)
apps/meteor/client/views/admin/ABAC/AdminABACTabs.tsx (1)
20-39: Permission-gated tab rendering looks correct.The conditional
TabsItemrendering is consistent with granular ABAC tab permissions and keeps navigation behavior intact.packages/i18n/src/locales/en.i18n.json (1)
94-101: ABAC granular permission i18n entries look consistent and complete.The new permission labels and matching
_descriptionentries are well-formed and aligned with the existing permissions localization pattern..changeset/brave-fans-tie.md (1)
2-6: Changeset entry is clear and correctly scoped.This captures the ABAC permission split and package version impact succinctly.
apps/meteor/client/views/admin/sidebarItems.ts (1)
71-78: ABAC sidebar gating logic looks correct.The combined requirement (
abac-management+ at least one granular tab permission) matches the new route/API authorization model.apps/meteor/client/views/admin/ABAC/AdminABACPage.tsx (1)
89-93: Tab and contextual content permission-gating is solid.Rendering now correctly follows granular permissions for page content and contextual bars.
Also applies to: 97-104
apps/meteor/client/views/admin/ABAC/hooks/useABACTabPermissions.ts (1)
3-13: Permission hook design is clean and consistent.The tab union, ordering, and permission map provide a clear single source of truth for ABAC tab authorization.
apps/meteor/client/views/admin/ABAC/AdminABACRoute.tsx (1)
24-27: Route-level granular permission handling is well implemented.Computing
firstAllowedTab, redirecting invalid tabs, and blocking access when no tab is allowed keeps navigation and auth behavior consistent.Also applies to: 31-40, 57-57
apps/meteor/ee/server/api/abac/index.ts (1)
49-50: Server-side granular ABAC enforcement is consistently applied.The per-endpoint permission split matches the intended tab/function boundaries and closes gaps where a single broad permission previously controlled everything.
Also applies to: 77-78, 100-101, 127-128, 154-155, 175-176, 206-207, 232-233, 256-257, 281-282, 300-301, 319-320, 337-338, 367-368, 399-400
apps/meteor/tests/end-to-end/api/abac.ts (1)
58-64: Granular permission test coverage is strong.These additions validate the new 403 behavior per missing permission and protect the auth matrix across ABAC endpoints.
Also applies to: 100-228, 2725-2731
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 10 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/meteor/client/views/admin/ABAC/AdminABACPage.tsx">
<violation number="1" location="apps/meteor/client/views/admin/ABAC/AdminABACPage.tsx:38">
P2: The sync button's `disabled` state only considers `isSyncDisabled` (LDAP/ABAC enabled flags), but the `/abac/users/sync` endpoint now also requires `manage-abac-admin-room-attributes`. A user with ABAC settings access but without the room-attributes permission will see an enabled button that always returns 403. Gate the button with `tabPermissions['room-attributes']` as well.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
3 participants
Proposed changes (including videos or screenshots)
Issue(s)
https://rocketchat.atlassian.net/browse/ABAC3-3
Steps to test or reproduce
Further comments
Summary by CodeRabbit